Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix class imports from node modules #165

Merged
merged 1 commit into from
Jan 7, 2022

Conversation

tylerferrara
Copy link
Collaborator

@tylerferrara tylerferrara commented Dec 23, 2021

Fixes #162

How import statements are currently treated

Let's use this simple export as an example:

// Foo.ts
import { DirectoryAddOptions } from "ts-morph";
export interface Foo {
  options: DirectoryAddOptions
}

For every export found within Foo.ts, the ts-auto-guard library will generate an accompanying guard for the exported variable. Regardless of how many exports are found, all guards are placed within the guard file: Foo.guard.ts.

Each property of the exported variable is analyzed to determine its type. In our example above, we only have one exported variable Foo which has a single property options. The expected guard file produced is as shown:

// Foo.guard.ts
/*
 * Generated type guards for "Foo.ts".
 * WARNING: Do not manually change this file.
 */
import { Foo } from "./Foo";

export function isFoo(obj: any, _argumentName?: string): obj is Foo {
    return (
        (obj !== null &&
            typeof obj === "object" ||
            typeof obj === "function") &&
        (obj.options !== null &&
            typeof obj.options === "object" ||
            typeof obj.options === "function") &&
        (typeof obj.options.recursive === "undefined" ||
            obj.options.recursive === false ||
            obj.options.recursive === true)
    )
}

NOTICE: There is no reference of DirectoryAddOptions within this guard file. That is because ts-auto-guard doesn't need it! It ONLY requires referencing the named type of a variable if it is a class. In our example, DirectoryAddOptions is an interface.

The Issue : #162

If we instead change the options variable to a class type, like Directory, we encounter some problems.

// Foo.ts
import { Directory } from "ts-morph";
export interface Foo {
  options: Directory
}
// Foo.guard.ts
/*
 * Generated type guards for "Foo.ts".
 * WARNING: Do not manually change this file.
 */
import { Directory } from "../node_modules/ts-morph/lib/ts-morph";
import { Foo } from "./Foo";

export function isFoo(obj: any, _argumentName?: string): obj is Foo {
    return (
        (obj !== null &&
            typeof obj === "object" ||
            typeof obj === "function") &&
        obj.options instanceof Directory
    )
}

As you can see, the guard file references the package ts-morph with a relative path within node_modules. This does not follow the TypeScript convention for non-relative package imports. In some cases, these relative imports do not correctly import the desired variable. This PR aims to address these problems with relative class imports.

The Fix

This PR adds a check for each import statement being added to the guard file. If the accompanying import is found within the node modules directory, it will explicitly copy the reference found within the source file. For our above example, we grab the package reference ts-morph found in Foo.ts. This solves our problem of referencing npm modules.

One alternative method, which was NOT used, was to copy the entire import statement found within the source file. However, this was discouraged as it has the potential to include more variables than necessary within the guard file. For example, a file like:

// Bar.js
import { Directory, DirectoryAddOptions } from "ts-morph";
console.log(DirectoryAddOptions)
export interface Bar {
  dir: Directory
}

will produce the guard file:

// Bar.guard.ts
/*
 * Generated type guards for "Foo.ts".
 * WARNING: Do not manually change this file.
 */
import { Directory, DirectoryAddOptions } from "ts-morph";
import { Bar } from "./Bar";

export function isBar(obj: any, _argumentName?: string): obj is Foo {
    return (
        (obj !== null &&
            typeof obj === "object" ||
            typeof obj === "function") &&
        obj.options instanceof Directory
    )
}

NOTICE: we import DirectoryAddOptions even though it is not used anywhere. This will raise an error for users with the TypeScript compiler option: noUnusedLocals. Therefore, I consider it a non-solution.

Testing

The current tests use an inMemoryFileSystem for generating test projects. However, this does not adequately simulate a project containing node-modules. Therefore, I created another test file to auto generate a local file for every test. This file ImportTest.ts is created and destroyed for every test to eliminate contamination between tests. It's written into the local directory /tests each time.

Since it lives in the local file system, within the local TypeScript project, we can adequately test class imports from npm packages already found in the ts-auto-guard project. The use of the ts-morph package for tests was chosen because this project already heavily relies on ts-morph types. Therefore, it is highly unlikely to be removed in the future, preventing test failures.

@rhys-vdw rhys-vdw added the do not merge This PR is not ready to be merged label Dec 25, 2021
@rhys-vdw
Copy link
Owner

@tylerferrara I've labeled this "do not merge" for now, let me know when you're ready and I'll give it a review. Thanks. :-)

@tylerferrara
Copy link
Collaborator Author

@tylerferrara I've labeled this "do not merge" for now, let me know when you're ready and I'll give it a review. Thanks. :-)

Fantastic! Just letting you know I didn't forget about this Issue. The fix appears to work as intended, just designing tests to prove it for a variety of scenarios.

Thank you for your patience 🙏

@tylerferrara tylerferrara changed the title [WIP] Imports fix Fix class imports from node modules Dec 28, 2021
@tylerferrara
Copy link
Collaborator Author

tylerferrara commented Dec 28, 2021

@rhys-vdw Ready for review

@rhys-vdw rhys-vdw removed the do not merge This PR is not ready to be merged label Dec 29, 2021
@rhys-vdw
Copy link
Owner

Hey @tylerferrara, I'm going to be a bit busy until next year but I'll review then. Looks sensible from a superficial reading but I'd like to understand it all better (esp. the test framework) before I merge. Thank you for your detailed contribution and enjoy your new year.

@rhys-vdw
Copy link
Owner

rhys-vdw commented Jan 7, 2022

Hey @tylerferrara hope you had a nice NYE. This is actually a very nice short PR. Awesome stuff. Certainly a bounty well earned. Typically I offer anyone who contributes to the project collaborator status, but since you're a bounty hunter I'm not sure if this appeals? In any case the invite is there should you wish to take it.

@rhys-vdw rhys-vdw merged commit 0fad41e into rhys-vdw:master Jan 7, 2022
@wiegell
Copy link

wiegell commented Jan 7, 2022

@tylerferrara throw a gitcoin submission whenever ready

@rhys-vdw
Copy link
Owner

rhys-vdw commented Jan 7, 2022

@wiegell thank you for your support of the project. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Declaration file is imported instead of actual class
3 participants